-
-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(npm): Allow to configure checkPackageName
for npm target
#504
Conversation
logger.warn('Adding tag "old" to not make it "latest" in registry.'); | ||
return 'old'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theoretically (not saying this should happen now), we could assign a <major>.x
tag here if we decide we want to go this route for multi-major version support.
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
const logger = { | ||
warn: jest.fn(), | ||
} as any; | ||
const actual = await getPublishTag( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm is this hitting actual public npm during test? we probably want to avoid that if possible since it makes these tests dependent on external state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this hits npm. I tried to write the test in a way that it works with whatever version we get back. But yes, if NPM would be down this would fail. Any idea how else to test this? 🤔 if we don't want to hit NPM the test also gets less realistic. The only thing I can think of is to mock spawnProcess
and have it return a static version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would record some example invocations of spawnProcess and patch them in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asottile-sentry I've updated the test to use a mocked process there instead!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This updates the
npm
target with the following new behavior:checkPackageName
is defined, we check the current version of this package on npmThis supersedes #502.